Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Site Editor: Only request templates and template parts for the current theme #27152

Closed
wants to merge 1 commit into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Nov 20, 2020

Description

Fixes #27149

  • Add a theme request parameter to wp_template REST request (identical to the existing one for wp_template_part) that allow to filter by theme slug (the built-in taxonomy filter only accepts term IDs).
  • Update all templates and template parts getEntityRecords in the Site Editor's navigation panel to only request template and template parts belonging to the current theme.

The main reason for this is to avoid confusion when using multiple FSE themes, exposing multiple, seemingly duplicate, templates and parts.

How has this been tested?

  • Have two FSE themes ready (e.g. "Twenty Twenty-One Blocks", and "Seedlet (Blocks)").
  • Activate the first, then the second.
  • Open /wp-admin/edit.php?post_type=wp_template and take note of all the templates with the same slug belonging to both themes (index is the best bet).
  • Open the Site Editor, then the navigation sidebar.
  • Navigate to the Templates menu.
  • Explore it and make sure there are no duplicates (or if you are able to tell, make sure the templates are only those for the current theme).
  • Check the Template Parts menu as well.
    The wp-admin menu for parts doesn't show auto-drafts and theme yet so you can't check there, but it should be enough to make sure there are no duplicates in the navigation sidebar.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

Size Change: -12 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/edit-site/index.js 23.1 kB -12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.96 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-library/index.js 148 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.93 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 9.58 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.45 kB 0 B
build/edit-post/style.css 6.44 kB 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.86 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.83 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.07 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand not wanting to show template/template-parts from OTHER themes, but why would we not want to show custom ones?

@Copons
Copy link
Contributor Author

Copons commented Nov 20, 2020

I understand not wanting to show template/template-parts from OTHER themes, but why would we not want to show custom ones?

@Addison-Stavlo new ones are created with the current theme set in the wp_theme taxonomy:

gutenberg/lib/templates.php

Lines 111 to 123 in 1862c62

/**
* Automatically set the theme meta for templates.
*
* @param array $post_id Template ID.
*/
function gutenberg_set_template_and_template_part_post_theme( $post_id ) {
$themes = wp_get_post_terms( $post_id, 'wp_theme' );
if ( ! $themes ) {
wp_set_post_terms( $post_id, array( wp_get_theme()->get_stylesheet() ), 'wp_theme', true );
}
}
add_action( 'save_post_wp_template', 'gutenberg_set_template_and_template_part_post_theme', 10, 3 );
add_action( 'save_post_wp_template_part', 'gutenberg_set_template_and_template_part_post_theme', 10, 3 );

@Addison-Stavlo
Copy link
Contributor

new ones are created with the current theme set in the wp_theme taxonomy:

I see. So custom template parts are then tied to a theme, which may not be ideal for users wanting to create custom template parts to use across whatever theme they choose to have active. 🤔

@Copons
Copy link
Contributor Author

Copons commented Nov 23, 2020

@Addison-Stavlo I'm sure this has been discussed to death already, but I'm failing at finding where. 🤔

My reasoning here is that having templates and parts always tied to a theme (and never showing other theme's items in the Site Editor), massively simplifies all the flows.
We don't have to worry about what happens when changing theme, for example.

Regardless, as far as I can see, we now have all the pieces (post status, theme and _wp_file_based tags) to build the puzzle as we see fit, so I think we wouldn't have a hard time changing directions either way.

For example, I can imagine a scenario where we update the theme tag if a user updates a template while using a different theme than the template's one.
Or, instead of updating the theme tag, we could insert a new one, so that templates would belong to multiple themes.
We could have give users the control, by introducing a UI to manually change the theme tag(s).

@Addison-Stavlo
Copy link
Contributor

I'm sure this has been discussed to death already, but I'm failing at finding where.

Im not sure if it has been actually, at least regarding whether custom created template parts should or shouldn't correspond to the theme that was active at the time of their creation. I think in the early stages of the site editor there wasn't much design involvement and a lot of the early development was going on a best guess of what we would need. The earliest stage I can remember regarding template part theme names behaved as: auto-drafts theme files get the theme name, wp-admin creation had the empty string, and in editor creation gave the user an empty input to put whatever they chose for the theme name. So coming in at that point I was assuming we had a need for having some separation between theme and custom and to not necessarily lump them together automatically under the same umbrella.

Given that the current state of the site editor best suits theme development, maybe this behavior isn't necessarily a bad thing for now. It does get fuzzy if we think of a user wanting to create a 'custom' type that is usable for their site regardless of any theme. 🤔 How much that makes sense in practice, I'm not sure 🤷‍♀️


For example, I can imagine a scenario where we update the theme tag if a user updates a template while using a different theme than the template's one.
We could have give users the control, by introducing a UI to manually change the theme tag(s).

But if we are only showing current theme in the site editor, they would have to navigate to wp-admin to do this? That fragments the experience quite a bit.

Anyways, with the way things are currently working I don't see any reason to block this PR. I do think we may need to rethink how/when we are adding the theme tag and that a _custom type may be necessary in the future.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works as expected.

Copy link
Member

@david-szabo97 david-szabo97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅ Works as expected!

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Jan 12, 2021

I think the end goal of this PR corresponds to the state we are now currently in as a result of #27910 ?

Im not sure if you want to close this or if there are still other issues to work out.

@Copons
Copy link
Contributor Author

Copons commented Jan 13, 2021

Thanks for checking this @Addison-Stavlo, I'll close it as not relevant anymore.

@Copons Copons closed this Jan 13, 2021
@Copons Copons deleted the update/fse-filter-templates-by-theme branch January 13, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site Editor: old templates appear in the sidebar
3 participants